-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated PT, modified V and added VT. #1
base: blackparrot_mods
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes! Fixes the problem of limited(2MB) user space.
Can we not change the v/* files for now though? I'm still working on debugging with the current version.
encoding.h
Outdated
#define PTE_PPN_SHIFT 10 | ||
#define PTE_PPN_OFFST 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of these constants can already be found in the header files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will look into it to find some replacement
v/vm.c
Outdated
// creating new page | ||
// get a new page from global ppgdir | ||
uint64_t newPage = alloc(); | ||
pt[0][VPN2(vaddr)] = ((newPage & ~0xfff) >> PTE_PPN_OFFST) | PTE_V; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the newpage's address should be asserted to be aligned with 4K pages instead of clearing the lower 12 bits.
v/vm.c
Outdated
@@ -296,5 +364,13 @@ void vm_boot(uintptr_t test_addr) | |||
trapframe_t tf; | |||
memset(&tf, 0, sizeof(tf)); | |||
tf.epc = test_addr - DRAM_BASE; | |||
|
|||
//user_llpt[VPN0(tf.epc)] = ((pte_t)test_addr/RISCV_PGSIZE << PTE_PPN_SHIFT) | PTE_V | PTE_R | PTE_W | PTE_X | PTE_A | PTE_D; | |||
for (long i = 0; i < usize; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before these changes, the user pages were not actually in the same location as the physical pages in the mem.
And on a page-fault, it copied those physical pages to the allocated user addresses.
I am not sure if one approach is better than the other, but in the old one, user mappings can be random and independent of how the code is loaded in the mem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly original way will allow us to test page fault.
encoding.h
Outdated
@@ -166,11 +167,19 @@ | |||
#define PTE_A 0x040 // Accessed | |||
#define PTE_D 0x080 // Dirty | |||
#define PTE_SOFT 0x300 // Reserved for Software | |||
|
|||
#define PTE_PPN 0x3ffffffffffc00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the PPN constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mask to extract physical page number from pte entry,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, give it a name that indicates that it’s a mask
v/vm.c
Outdated
freelist_head = freelist_tail = 0; | ||
return res; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem like substantial changes to how the VM works. @farzamgl is all this necessary to support the 2MB userspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code generalizes the page-table generation so we can have more than 2MB space. It's not that different except the way that the mappings are done, and there is no page copying.
In the original code, there is only 1 entry in L2 page-table for each user and kernel code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the vt/*, there will be user code copying and remapping when a timer interrupt is taken.
v/vm.c
Outdated
pte_t* second_pte_ptr; | ||
pte_t second_pte; | ||
pte_t* third_pte_ptr; | ||
pte_t third_pte; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need separate ptr variables, just use & operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
& operator on local variables does not return address to original page table. To modify original pte value, I belive a pointer is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can go the reverse way if necessary. The point is you don’t need a variable for both the pointer and the pointer contents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Your way is cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farzamgl Yes, I will leave v/* unchanged and put all my changes into vt/*.
vt/* utilizes timer interrupts to make remapping more frequent.
addi a0, a0, TIMER_INTERVAL; \ | ||
csrw mtimecmp, a0; \ | ||
csrr t0, mstatus; \ | ||
ori t0, t0, MSTATUS_MPIE; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing PIE here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on mret. mie in mstatus will become mpie in mstatus. mie in mstatus is the global interrupt enable signal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s done in hardware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If I do not write MPIE before mret, MIE will be clearer after mret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it won’t. On the timer interrupt, MPIE becomes MIE (1) in hardware. On MRET, MIE becomes MPIE (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because on MRET, MIE becomes MPIE. I write MPIE to 1 to before MRET to enable timer interrupt after MRE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re not understanding. MPIE is already 1 when you’re in the trap. If it weren’t, you wouldn’t have taken the trap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at how the old spike code worked. They didn’t need to write to MPIE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MPIE is only for initialization. During reset mstatus was all zeros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert v/*
to the version on blackparrot_mods
not master
c49a9a0
to
d8f777c
Compare
Change mtime, mtimecmp csr accesses to memory mapped loads and stores. Added env VT(virtual timer interrupt)